Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis pull request refactors the CLI statistics commands by creating a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
test/unit/commands/help-consistency.test.ts (1)
144-149: LGTM! Test correctly updated for new command structure.Removing "stats" from the expected accounts subcommands is correct, as
statsis now a top-level command group rather than a subcommand ofaccounts. The test appropriately validates that only["current", "list", "login", "logout", "switch"]remain under the accounts topic.Consider adding a similar consistency test for the new
statstopic command to ensure its help output is consistent across interactive and non-interactive modes:// In a new test or existing test file it("should show consistent help for stats command", async () => { const topicsWithCommands = ["accounts", "apps", "channels", "stats"]; for (const topic of topicsWithCommands) { const { stdout } = await execAsync(`node ${binPath} ${topic} --help`); expect(stdout).toContain("COMMANDS"); } });
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Disabled knowledge base sources:
- Jira integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (13)
README.mddocs/Product-Requirements.mddocs/TODO.mdsrc/commands/accounts/index.tssrc/commands/accounts/stats/README.mdsrc/commands/apps/index.tssrc/commands/connections/index.tssrc/commands/connections/stats.tssrc/commands/stats/account.tssrc/commands/stats/app.tssrc/commands/stats/index.tstest/unit/commands/connections/stats.test.tstest/unit/commands/help-consistency.test.ts
💤 Files with no reviewable changes (6)
- src/commands/connections/stats.ts
- src/commands/connections/index.ts
- test/unit/commands/connections/stats.test.ts
- src/commands/accounts/index.ts
- src/commands/apps/index.ts
- src/commands/accounts/stats/README.md
🧰 Additional context used
📓 Path-based instructions (7)
src/commands/**/*.ts
📄 CodeRabbit inference engine (.cursor/rules/AI-Assistance.mdc)
src/commands/**/*.ts: Commands should follow the oclif structure: export default class MyCommand extends Command { ... }
Command should support auth via the standard auth helper (import { getAuth } from '../../helpers/auth') and use it in the run method.
Use try/catch for error handling in commands, and provide user-friendly error messages (e.g., handle error.code === 40100 for authentication failures).
Do not use direct HTTP requests (e.g., fetch) for data plane APIs; use the Ably SDK instead.
Do not use hardcoded endpoint URLs; use configuration values (e.g., flags.controlHost, config.controlHost) when constructing URLs.
Do not use non-standard command structures (e.g., export default class { async execute(args) { ... } }); always use the oclif Command class.
Files:
src/commands/stats/index.tssrc/commands/stats/app.tssrc/commands/stats/account.ts
**/*.{ts,tsx}
📄 CodeRabbit inference engine (.cursor/rules/Development.mdc)
**/*.{ts,tsx}: Use TypeScript and follow best practice naming conventions
ESLint is used to ensure all code adheres best practices; ensure you check that all code passes the linter before concluding your work.
Files:
src/commands/stats/index.tstest/unit/commands/help-consistency.test.tssrc/commands/stats/app.tssrc/commands/stats/account.ts
src/commands/**/*.{ts,js}
📄 CodeRabbit inference engine (.cursor/rules/Development.mdc)
Follow oclif framework best practices as described in the oclif documentation.
Files:
src/commands/stats/index.tssrc/commands/stats/app.tssrc/commands/stats/account.ts
**/*.test.ts
📄 CodeRabbit inference engine (.cursor/rules/AI-Assistance.mdc)
**/*.test.ts: When testing, mock the Ably SDK properly by stubbing the constructor (e.g., sinon.stub(Ably, 'Rest').returns(mockClient)).
Always ensure resources are properly closed/cleaned up in tests (e.g., call await client.close() and sinon.restore() in afterEach).
Files:
test/unit/commands/help-consistency.test.ts
{test/**/*.{ts,js},**/*.{test,spec}.{ts,js}}
📄 CodeRabbit inference engine (.cursor/rules/Development.mdc)
When new features are added or changes made, tests must be updated or added, and it is your responsibility to ensure the tests pass before deeming your work complete.
Files:
test/unit/commands/help-consistency.test.ts
{.cursor/rules/**,docs/**}
📄 CodeRabbit inference engine (.cursor/rules/Development.mdc)
Whenever new features or changes are made, you must look at the .cursor/rules and /docs and update them to reflect any changes made.
Files:
docs/TODO.mddocs/Product-Requirements.md
{README.md,docs/**,.cursor/rules/**}
📄 CodeRabbit inference engine (.cursor/rules/Development.mdc)
Update all relevant documentation, including README.md, files within the docs/ directory, and files within the .cursor/rules/ directory if the changes impact requirements, structure, testing procedures, development guidelines, Ably usage, or project structure.
Files:
docs/TODO.mddocs/Product-Requirements.mdREADME.md
🪛 markdownlint-cli2 (0.18.1)
README.md
819-819: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
2369-2369: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
2406-2406: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
2443-2443: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
2476-2476: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
2499-2499: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
2672-2672: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
3421-3421: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
3854-3854: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Cursor Bugbot
- GitHub Check: auth-tests
- GitHub Check: ui-tests
- GitHub Check: session-tests
- GitHub Check: e2e-cli
- GitHub Check: test
- GitHub Check: Cursor Bugbot
🔇 Additional comments (8)
src/commands/stats/index.ts (1)
1-17: LGTM! Well-structured topic command.The new
StatsCommandclass properly extendsBaseTopicCommandand provides clear examples demonstrating bothstats accountandstats appsubcommands with various options. This establishes the new top-level stats command group as intended.docs/Product-Requirements.md (2)
159-164: LGTM! Documentation accurately reflects the new command structure.The new Statistics section clearly documents the
ably stats accountandably stats appcommands with their supported flags and functionality. The descriptions align with the implementation.
323-323: LGTM! MCP command reference updated correctly.The MCP commands list has been updated from
ably apps statstoably stats app, maintaining consistency with the refactored command structure.src/commands/stats/account.ts (1)
76-83: LGTM! Proper configuration for account-level stats.The addition of
isAccountStats: truecorrectly distinguishes account stats from app stats in theStatsDisplayconfiguration, allowing the display service to handle account-specific formatting appropriately.docs/TODO.md (1)
75-81: LGTM: Stats section properly reorganized.The new
Statssection with dedicatedstats accountandstats appsubtasks correctly reflects the PR objectives. Test coverage items (parsing/formatting and live-polling) are well-defined and consistent with the overall document structure.README.md (3)
96-96: Potential inconsistency: Stats commands documentation location after refactoring.Per the PR objectives,
ably accounts statsshould be migrated toably stats account(top-level stats group). However, the README still documents both:
- Line 96:
ably accounts statsin the accounts command index- Lines 410–462: Full
ably accounts statscommand documentation- Lines 1038–1099: Full
ably apps statscommand documentationIf this PR moves these commands to a new top-level
ably statsgroup, these sections should either be removed or replaced with references to the new top-level stats commands. The newably stats accountandably stats appdocumentation should also be present in the README.Verify that:
- The new top-level
ably stats accountandably stats appcommands are documented in this README- The old
ably accounts statsandably apps statssections should be removed if migration is complete- If backward compatibility aliases exist, clarify whether these old commands are still supported
Also applies to: 410-462, 1038-1099
2472-2530: LGTM: Connections section correctly reflects stats command removal.The
ably connectionssection correctly removes theconnections statscommand reference (per line 2489 showing onlylogsandtestcommands remain). This aligns with the PR objective to removeably connections stats.
106-106: Clarify scope: Parameter naming changes (APPID, INTEGRATIONID, QUEUEID).Multiple parameter names have been updated in this PR (e.g.,
[APPID]at line 106,INTEGRATIONIDat line 2668,QUEUEIDat line 3417). While these improvements clarify the argument semantics, verify they are:
- Intentional improvements aligned with the PR scope, or
- Incidental changes that should be separated into a different PR for clarity
These changes are not explicitly mentioned in the PR objectives (which focus on stats command reorganization), but they appear to be beneficial documentation clarifications. Confirm these align with your development workflow.
Verify these parameter naming changes are intentional within this PR's scope.
Also applies to: 112-112, 153-153, 175-175, 186-186, 2668-2668, 3417-3417, 3427-3427
|
|
||
| Delete an app | ||
|
|
||
| ``` |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check the size of README.md
wc -l README.mdRepository: ably/ably-cli
Length of output: 71
🏁 Script executed:
# Check for markdownlint configuration
find . -maxdepth 2 -type f \( -name ".markdownlint*" -o -name "*.md" -path "*/.markdownlint*" \) | head -20Repository: ably/ably-cli
Length of output: 39
🏁 Script executed:
# Check for validation/linting scripts in package.json or similar
if [ -f package.json ]; then cat package.json | grep -A 20 '"scripts"'; fi
if [ -f pnpm-workspace.yaml ]; then cat pnpm-workspace.yaml | head -50; fi
if [ -f .editorconfig ]; then cat .editorconfig | head -50; fiRepository: ably/ably-cli
Length of output: 1610
🏁 Script executed:
# Check if markdownlint-cli2 or markdownlint is installed
if [ -f package.json ]; then cat package.json | grep -i markdown; fi
if [ -f package-lock.json ]; then grep -i "markdownlint" package-lock.json | head -5; fi
if [ -f pnpm-lock.yaml ]; then grep -i "markdownlint" pnpm-lock.yaml | head -5; fiRepository: ably/ably-cli
Length of output: 39
🏁 Script executed:
# Read the specific lines with context - checking each area for language specifiers
echo "=== Line 819 context ===" && sed -n '815,825p' README.md
echo "=== Line 2369 context ===" && sed -n '2365,2375p' README.md
echo "=== Line 2406 context ===" && sed -n '2402,2412p' README.md
echo "=== Line 2443 context ===" && sed -n '2439,2449p' README.md
echo "=== Line 2476 context ===" && sed -n '2472,2482p' README.md
echo "=== Line 2499 context ===" && sed -n '2495,2505p' README.md
echo "=== Line 2672 context ===" && sed -n '2668,2678p' README.md
echo "=== Line 3421 context ===" && sed -n '3417,3427p' README.md
echo "=== Line 3854 context ===" && sed -n '3850,3860p' README.mdRepository: ably/ably-cli
Length of output: 3307
🏁 Script executed:
# Search for any markdownlint configuration in the repo
find . -type f \( -name ".markdownlint*" -o -name "markdownlint*" \) 2>/dev/null | head -20Repository: ably/ably-cli
Length of output: 39
🏁 Script executed:
# Check if markdownlint is mentioned in CI/CD files
find . -type f \( -name ".github" -o -name ".gitlab-ci.yml" -o -name ".circleci" \) 2>/dev/null
find .github -type f -name "*.yml" -o -name "*.yaml" 2>/dev/null | xargs grep -l markdown 2>/dev/null | head -10Repository: ably/ably-cli
Length of output: 39
🏁 Script executed:
# Count fenced code blocks without language specifiers in README.md
# This finds lines that start with exactly 3 backticks followed by newline (no language specifier)
grep -n "^\`\`\`$" README.md | wc -lRepository: ably/ably-cli
Length of output: 60
🏁 Script executed:
# Also check the total count and see examples
echo "Total code blocks without language specifier:"
grep -n "^\`\`\`$" README.md | head -20Repository: ably/ably-cli
Length of output: 260
Add language specifiers to fenced code blocks in README.md to improve documentation quality and markdown linting compliance.
The README.md contains 267 fenced code blocks without language specifiers. The examples provided (lines 819, 2369, 2406, 2443, 2476, 2499, 2672, 3421, 3854) show code blocks that should be marked with shell or other appropriate language identifiers. Per coding guidelines, documentation updates must maintain quality standards—all code blocks should include proper language tags for consistency.
🔎 Example fix for code block language specifiers
For shell commands:
-```
+```shell
USAGE
$ ably config path
For structured data/JSON:
```diff
-```
+```json
{...}
</details>
> Committable suggestion skipped: line range outside the PR's diff.
<details>
<summary>🧰 Tools</summary>
<details>
<summary>🪛 markdownlint-cli2 (0.18.1)</summary>
819-819: Fenced code blocks should have a language specified
(MD040, fenced-code-language)
</details>
</details>
<!-- fingerprinting:phantom:triton:mongoose -->
<!-- This is an auto-generated comment by CodeRabbit -->
| import { ControlBaseCommand } from "../../control-base-command.js"; | ||
| import { StatsDisplay } from "../../services/stats-display.js"; | ||
| import type { BaseFlags } from "../../types/cli.js"; | ||
| import type { ControlApi } from "../../services/control-api.js"; | ||
|
|
||
| export default class AccountsStatsCommand extends ControlBaseCommand { | ||
| export default class StatsAccountCommand extends ControlBaseCommand { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for old command references
echo "=== Searching for 'accounts stats' references ==="
rg -n "accounts stats" --type md --type ts --type json -g '!CHANGELOG*' 2>/dev/null || echo "No matches found"
echo -e "\n=== Searching for old class name 'AccountsStatsCommand' ==="
rg -n "AccountsStatsCommand" --type ts 2>/dev/null || echo "No matches found"
echo -e "\n=== Searching for old import paths 'commands/accounts/stats' ==="
rg -n "commands/accounts/stats" --type ts 2>/dev/null || echo "No matches found"
echo -e "\n=== Checking the actual file content ==="
cat -n src/commands/stats/account.ts | head -100Repository: ably/ably-cli
Length of output: 5147
Update README.md documentation to reflect the new command structure.
While the code refactoring to StatsAccountCommand is correct and internally consistent, the README.md documentation has not been updated and still references the old ably accounts stats command structure throughout (lines 96, 244, 251, 410, 416, 445, 447, 449, 451, 453, 455, 457, 459). All examples and command descriptions in the README must be updated to use ably stats account to match the refactored code.
🤖 Prompt for AI Agents
In src/commands/stats/account.ts around lines 4 to 9, the code rename/refactor
to StatsAccountCommand changed the CLI command namespace but README.md still
references the old "ably accounts stats" usage; update README.md at the listed
lines (96, 244, 251, 410, 416, 445, 447, 449, 451, 453, 455, 457, 459) to
replace occurrences of "ably accounts stats" (and any example invocations or
descriptions) with the new "ably stats account" form, ensuring examples, help
text, and any command usage snippets match the refactored command structure and
adjust surrounding wording if needed for grammar or flags consistency.
| import { ControlBaseCommand } from "../../control-base-command.js"; | ||
| import { StatsDisplay } from "../../services/stats-display.js"; | ||
| import type { BaseFlags } from "../../types/cli.js"; | ||
| import type { ControlApi } from "../../services/control-api.js"; | ||
|
|
||
| export default class StatsAppCommand extends ControlBaseCommand { |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# First, let's read the actual file to confirm current state
echo "=== Reading src/commands/stats/app.ts ==="
cat -n src/commands/stats/app.ts | head -80
echo -e "\n=== File size check ==="
wc -l src/commands/stats/app.tsRepository: ably/ably-cli
Length of output: 3319
🏁 Script executed:
#!/bin/bash
# Search for old command references that might have been missed
echo "=== Searching for 'apps stats' references ==="
rg "apps stats" --type ts --type md --type json -i
echo -e "\n=== Searching for old class name 'AppsStatsCommand' ==="
rg "AppsStatsCommand" --type ts
echo -e "\n=== Searching for old import path 'commands/apps/stats' ==="
rg "commands/apps/stats" --type ts --type jsonRepository: ably/ably-cli
Length of output: 1102
🏁 Script executed:
#!/bin/bash
# Check if old file location still exists
echo "=== Checking if old file location exists ==="
if [ -f "src/commands/apps/stats.ts" ]; then
echo "OLD FILE STILL EXISTS: src/commands/apps/stats.ts"
wc -l src/commands/apps/stats.ts
else
echo "Old file location does not exist (expected)"
fi
# Check what's in the apps directory
echo -e "\n=== Contents of src/commands/apps/ ==="
find src/commands/apps -type f 2>/dev/null || echo "Directory does not exist"Repository: ably/ably-cli
Length of output: 722
Update README.md documentation to reflect the new command path.
The refactoring of src/commands/stats/app.ts is correct: the class is properly named StatsAppCommand, import paths are updated, and examples use the new ably stats app format. However, the README.md file still contains 20+ references to the old ably apps stats command structure that must be updated to match the new ably stats app command path.
🤖 Prompt for AI Agents
In src/commands/stats/app.ts around lines 4 to 9, the command was refactored to
StatsAppCommand and the CLI path changed to "ably stats app", but README.md
still references the old "ably apps stats" path; update README.md by searching
for all occurrences of "ably apps stats" (and variants like "ably apps stat" or
examples using that order) and replace them with "ably stats app", adjust
example command blocks and any usage notes to match the new command syntax, and
run a quick docs build or grep to confirm no remaining references.
vladvelici
force-pushed
the
stats-command-FTF-262
branch
from
0a1d085 to
4145a91
Compare
vladvelici
force-pushed
the
stats-command-FTF-262
branch
from
97c3ec4 to
0d6411f
Compare
| import { ControlBaseCommand } from "../../control-base-command.js"; | ||
| import { StatsDisplay } from "../../services/stats-display.js"; | ||
| import type { BaseFlags } from "../../types/cli.js"; | ||
| import type { ControlApi } from "../../services/control-api.js"; |
There was a problem hiding this comment.
Ditto coderabbit on removing references in README etc to account stats and app stats
| }); | ||
|
|
||
| describe("Error Handling E2E", () => { | ||
| it( |
There was a problem hiding this comment.
Is there another command we could use rather than removing the entire test?
There was a problem hiding this comment.
Nothing else seems to take a unit in connections. We only have connections test left and it doesn't take --unit.
There was a problem hiding this comment.
Ended up making an e2e tests group for stats instead, let me know what you think.
vladvelici
force-pushed
the
stats-command-FTF-262
branch
from
7764176 to
b0684e6
Compare
test/e2e/stats/stats.test.ts
Outdated
| await cleanupTrackedResources(); | ||
| }); | ||
|
|
||
| describe("stats account", () => { |
There was a problem hiding this comment.
If the intention in beforeAll is to skip tests if E2E is disabled, you can use describe.skipIf - what we currently have will just short-circuit the beforeAll, not stop tests running.
There was a problem hiding this comment.
good catch, fixed
There was a problem hiding this comment.
Lots of e2e tests have different skipping patterns, I didn't go for a wider fix in this PR. Happy to look at it next, and move all e2e tests to the skipIf?
vladvelici
force-pushed
the
stats-command-FTF-262
branch
from
b0684e6 to
380bee4
Compare
vladvelici
force-pushed
the
stats-command-FTF-262
branch
from
380bee4 to
9cb539e
Compare
umair-ably
force-pushed
the
stats-command-FTF-262
branch
from
9cb539e to
e021120
Compare
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
|
(One comment also agreeing w/ what Cursorbot said) |
There was a problem hiding this comment.
Pull request overview
This PR reorganizes the CLI statistics surface area by introducing a new top-level ably stats topic (with stats account and stats app [id]) and removing legacy accounts/apps/connections stats entry points, with corresponding updates to help/autocomplete, docs, and E2E coverage.
Changes:
- Introduces
src/commands/stats/*and updates interactive help/autocomplete expectations to surfacestatsas a top-level topic. - Removes the legacy
connections statscommand implementation and its associated unit/E2E coverage. - Refreshes README command docs and adds MCP server documentation; adds a new comprehensive stats E2E test suite.
Reviewed changes
Copilot reviewed 18 out of 18 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| test/unit/help/interactive-help.test.ts | Updates interactive help filtering expectations to avoid removed accounts:stats. |
| test/unit/commands/interactive-autocomplete.test.ts | Updates autocomplete command list to include top-level stats. |
| test/unit/commands/help-consistency.test.ts | Removes accounts stats from expected help output consistency checks. |
| test/unit/commands/connections/stats.test.ts | Deletes unit tests for the removed connections:stats command. |
| test/unit/base/base-command.test.ts | Updates anonymous web CLI restriction test case away from removed accounts:stats. |
| test/e2e/stats/stats.test.ts | Adds new E2E coverage for stats account and stats app, including live mode and error cases. |
| test/e2e/connections/connections.test.ts | Removes E2E coverage for connections stats now that it’s deleted. |
| src/hooks/init/alias-command.ts | Updates hook comment example after stats command reorg. |
| src/commands/stats/index.ts | Adds new stats top-level topic command. |
| src/commands/stats/app.ts | Moves/renames app stats command under the new stats topic. |
| src/commands/stats/account.ts | Moves/renames account stats command under the new stats topic. |
| src/commands/connections/stats.ts | Deletes the legacy connections stats implementation. |
| src/commands/connections/index.ts | Removes stats from connections topic examples. |
| src/commands/apps/index.ts | Removes stats from apps topic examples. |
| src/commands/accounts/stats/README.md | Deletes legacy docs for accounts stats. |
| src/commands/accounts/index.ts | Removes stats from accounts topic examples. |
| src/base-command.ts | Updates comment regarding web CLI restrictions vs new stats command id(s). |
| README.md | Updates command docs and adds MCP sections; reflects new stats command structure. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| running command... | ||
| $ ably (--version) | ||
| @ably/cli/0.16.0 darwin-arm64 node-v22.14.0 | ||
| @ably/cli/0.15.0 darwin-arm64 node-v22.14.0 |
There was a problem hiding this comment.
The README’s generated usage snippet shows version 0.15.0, but package.json is 0.16.0. This also appears to have cascaded into many “See code” links pointing at blob/v0.15.0. Please regenerate the README (e.g., via oclif readme) after confirming the correct package version/tag so the displayed version and GitHub links match the current release.
| @ably/cli/0.15.0 darwin-arm64 node-v22.14.0 | |
| @ably/cli/0.16.0 darwin-arm64 node-v22.14.0 |
There was a problem hiding this comment.
@coderabbitai can you cross- check if this is correct?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
cat package.json | grep '"version"' | head -5
Length of output: 74
@sacOO7, the Copilot comment is correct.
package.json declares version 0.16.0, but this PR's diff is actually downgrading the displayed version in the README usage snippet from 0.16.0 → 0.15.0:
-@ably/cli/0.16.0 darwin-arm64 node-v22.14.0
+@ably/cli/0.15.0 darwin-arm64 node-v22.14.0This looks like the README was regenerated from an older state or a stale local install. The same mismatch cascades into all the _See code_ links throughout the file, which currently point to blob/v0.15.0 instead of blob/v0.16.0.
The fix is to regenerate the README against the current codebase:
oclif readme
git add README.mdThis will bring the version string and all source links back in sync with the 0.16.0 declared in package.json.
@AndyTWF should be a-okay now |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 18 out of 18 changed files in this pull request and generated 3 comments.
Comments suppressed due to low confidence (1)
test/unit/help/interactive-help.test.ts:229
WEB_CLI_ANONYMOUS_RESTRICTED_COMMANDSnow includes the newstats*wildcard, but this wildcard-pattern test doesn’t assert the expected hiding behavior forstats/stats:account/stats:app. Adding at least onestats*assertion here would prevent regressions in anonymous-mode help filtering for the newly introduced top-level stats topic.
// Test wildcard patterns
expect(help.shouldDisplay({ id: "accounts" } as any)).toBe(false); // matches accounts*
expect(help.shouldDisplay({ id: "accounts:current" } as any)).toBe(false); // matches accounts*
expect(help.shouldDisplay({ id: "apps" } as any)).toBe(false); // matches apps*
expect(help.shouldDisplay({ id: "apps:current" } as any)).toBe(false); // matches apps*
expect(help.shouldDisplay({ id: "bench" } as any)).toBe(false); // matches bench*
expect(help.shouldDisplay({ id: "bench:subscriber" } as any)).toBe(false); // matches bench*
expect(help.shouldDisplay({ id: "logs" } as any)).toBe(false); // matches logs*
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| const cliPath = join(process.cwd(), "bin", "run.js"); | ||
|
|
||
| // Start live stats subscription | ||
| const statsMonitor = spawn( | ||
| "node", | ||
| [ | ||
| cliPath, | ||
| "stats", | ||
| "account", | ||
| "--live", | ||
| "--interval", | ||
| "2", | ||
| "--access-token", | ||
| E2E_ACCESS_TOKEN!, | ||
| ], | ||
| { | ||
| env: { | ||
| ...process.env, | ||
| }, | ||
| }, | ||
| ); | ||
|
|
||
| let output = ""; | ||
| statsMonitor.stdout?.on("data", (data) => { | ||
| output += data.toString(); | ||
| }); | ||
| statsMonitor.stderr?.on("data", (data) => { | ||
| output += data.toString(); | ||
| }); | ||
|
|
||
| // Wait for some output | ||
| await new Promise((resolve) => setTimeout(resolve, 8000)); | ||
|
|
||
| // Gracefully terminate | ||
| statsMonitor.kill("SIGTERM"); | ||
|
|
||
| await new Promise<void>((resolve) => { | ||
| statsMonitor.on("exit", () => resolve()); | ||
| setTimeout(resolve, 5000); // Fallback timeout | ||
| }); |
There was a problem hiding this comment.
The live-mode test uses a raw spawn() and then resolves after a fixed fallback timeout even if the child process hasn’t actually exited. This can leave a node bin/run.js stats account --live ... process running, making the E2E suite flaky/hang. Prefer using the existing CliRunner/startCli helpers (which track/cleanup processes) or, at minimum, (1) track the spawned PID for global cleanup, (2) fail if the process hasn’t exited after the grace period, and (3) escalate to SIGKILL and await exit before asserting on output.
| await new Promise<void>((resolve) => { | ||
| statsMonitor.on("exit", () => resolve()); | ||
| setTimeout(resolve, 5000); // Fallback timeout |
There was a problem hiding this comment.
Same issue as the account live-mode test: this test resolves after a fallback timeout without guaranteeing the stats app --live child process has exited (and the process isn’t registered for global cleanup). This risks leaving orphaned processes that keep the test runner alive. Please use the shared CLI runner helpers or add robust termination (track PID, enforce exit, escalate to SIGKILL if needed) before continuing.
| await new Promise<void>((resolve) => { | |
| statsMonitor.on("exit", () => resolve()); | |
| setTimeout(resolve, 5000); // Fallback timeout | |
| let exited = false; | |
| await new Promise<void>((resolve) => { | |
| const timeout = setTimeout(() => { | |
| if (!exited) { | |
| // Escalate to SIGKILL if the process has not exited in time | |
| try { | |
| statsMonitor.kill("SIGKILL"); | |
| } catch { | |
| // Ignore errors from killing an already-exited process | |
| } | |
| } | |
| resolve(); | |
| }, 5000); | |
| statsMonitor.on("exit", () => { | |
| exited = true; | |
| clearTimeout(timeout); | |
| resolve(); | |
| }); |
There was a problem hiding this comment.
When I run
ably stats app
I get
> @ably/cli@0.16.0 cli /Users/sachinsh/JsProjects/ably-cli
> bin/run.js stats app
Fetching stats for app jy3uew...
No stats found for the specified period
For ably stats app, appId can also be specified
ably stats app app-id
For
ably stats account
I get
> @ably/cli@0.16.0 cli /Users/sachinsh/JsProjects/ably-cli
> bin/run.js stats account
No stats found for the specified period
Since, we can't specify accountId here (as per --help doc), logs could be more explicit about the account context, since it shows stats only for the current account, i.e ably accounts current
P.S. Not sure whether we should also allow accountName or accountId to be passed as a parameter here.
sacOO7
left a comment
sacOO7
left a comment
There was a problem hiding this comment.
Apart from comment at #122 (review), LGTM
@sacOO7 addressed in the latest commit |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Free Tier Details
Your team is on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle for each member of your team.
To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.
Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
https://ably.atlassian.net/browse/FTF-262
Note
Medium Risk
Renames/removes several CLI commands (notably
accounts stats,apps stats, andconnections stats) in favor of newably stats account|app, which can break existing scripts and user workflows. Adds new experimental MCP server surface area and updates help/autocomplete/tests, so regressions are possible in CLI UX and command routing.Overview
CLI surface reorg: Introduces a new top-level
ably statstopic withstats accountandstats app [id], and removes the legacyaccounts stats,apps stats, andconnections statscommands (including deleting theconnections statsimplementation and related docs/examples).Docs/tests refreshed + MCP additions: Updates
README.mdcommand list/usage text (including new MCP usage/server sections andably mcp start-serverhelp), adjusts web/anonymous restriction expectations and interactive help/autocomplete listings, and replaces prior connection-stats coverage with a new, comprehensivestatsE2E test suite.Written by Cursor Bugbot for commit e021120. This will update automatically on new commits. Configure here.
Summary by CodeRabbit
Refactor
statstopic for improved command structure (e.g.,stats account,stats appreplaceaccounts statsandapps stats). Removed theconnections statscommand.Documentation
✏️ Tip: You can customize this high-level summary in your review settings.